Description

This adds timeouts to all wallet API calls (sendPayment, testSendPayment, testCreateInvoice and createInvoice). The timeouts are defined as constants (WALLET_CREATE_INVOICE_TIMEOUT_MS and WALLET_SEND_PAYMENT_TIMEOUT_MS). These timeouts fix the unresponsiveness as mentioned in #1558:

Wallet payments can fail silently, e.g. the NWC consumer doesn't respond to a payment request, in which case we will wait until the invoice expires (10 minutes in most cases) - which is way too long. Stackers will assume we're unresponsive.

#1558 only mentioned this for payments but I think we also want timeouts for creating invoices since they also affect responsiveness.

Regarding this:

To timeout safely, we'll want to cancel the original invoice, then generate a new invoice to display in the QR.

This means TimeoutError needs to be considered a WalletPaymentError for sender fallbacks (#1642) to work as expected. That is the case since we already throw any error as a WalletSenderError in useSendPayment.

Additionally to wrapping every wallet API call with withTimeout, we also pass an AbortSignal which is passed to any fetch call inside the wallet API to abort any pending request on timeout. This is important to prevent any further logging after timeout. We need both (withTimeout and AbortSignal) because not every wallet uses fetch but some use gRPC, websockets which don't have built-in support for signals.

close #1558 based on #1724 #1725 #1726

TODO

• pass signal to abort any pending fetch and prevent confusing logging after timeout

Additional Context

• I am considering to also pass and use an AbortSignal instance to all wallet API calls. It can then be passed to any fetch call so they also get aborted on timeout. Without this, requests will finish even though we don't need the response anymore. This has probably not a lot of impact on UX or performance but it just feels like a nice thing to do. It might not even be hard to do if it's really just passing signal to fetch.

update: yes, this is pretty important since else we will continue to log stuff inside the wallet API even though we already logged a timeout error. Added as TODO. The wallets that don't support signals don't log currently but if they ever do, we can also pass the signal to the logger calls to check if it's aborted.

• I am also considering to make all API calls during testSendPayment and testCreateInvoice warnings in another PR so we don't need to worry too much about too short timeouts causing errors during attachment. Maybe it's also good if they are the same as the real timeouts since they give immediate feedback (as warnings) about the wallet response time ("health of the wallet")?

• I mentioned that we could use short invoice expiration in Timeout wallet payments #1558 (comment) but since the required cancel stuff was already handled in Sender fallbacks #1642, simply timing out the calls is enough.

• If a receiver has multiple wallets, the timeout is currently applied to each wallet individually. This means that the full timeout to create an invoice can be multiples of WALLET_CREATE_INVOICE_TIMEOUT_MS. Maybe we should divide the timeout by the amount of wallets so each wallet gets the same time to create an invoice while still not waiting too long for an invoice from any wallet?

• node-fetch doesn't throw the custom error we passed into controller.abort¹ so for CLN and LNbits, I didn't use fetchWithTimeout but custom error handling. We use node-fetch because of the https agent support.

Checklist

Are your changes backwards compatible? Please answer below:

yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

8. Tested by attaching every wallet (send+recv) except LNC and WebLN.